Skip to content

[java] assert setFiles uploads reach the server in SetFilesCommandTest - #18058

Merged
diemol merged 2 commits into
SeleniumHQ:trunkfrom
yashp676:java-setfiles-upload-roundtrip
Oct 6, 2026
Merged

diemol merged 2 commits into
SeleniumHQ:trunkfrom
yashp676:java-setfiles-upload-roundtrip

Conversation

@yashp676

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds two round-trip tests to SetFilesCommandTest. The existing four only check
that the input's value ends with the filename, which doesn't prove the file was
uploaded. The new ones submit the form and check what the upload endpoint echoed
back into the iframe, the same way UploadTest does.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@selenium-ci selenium-ci added C-java Java Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Sep 20, 2026
@yashp676

Copy link
Copy Markdown
Contributor Author

@pujagani would you have time to look at this? @titusfortner pointed me your way for Java BiDi. #18025 is also open if the UploadHandler parsing change is in your area — happy to split it differently if the two would be easier to review separately.

@yashp676

yashp676 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Hi @diemol and @titusfortner now that #18025 has been merged, this is the follow-up from #18010: two round-trip tests in SetFilesCommandTest that submit the form and assert the uploaded contents reach the server, the same way UploadTest does. It's test-only, 80 lines in one file.Could one of you take a look when you have a moment? Thanks!

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Safari upload checks fail in CI 🐞 Bug ≡ Correctness
Description
The two new round-trip methods require exact text from the upload iframe without marking Safari as
not yet implemented. Safari returns incorrect frame-body text for this upload flow, so both tests
fail on Safari instead of being handled as known unsupported coverage.
Code

java/test/org/openqa/selenium/bidi/input/SetFilesCommandTest.java[R131-134]

+  @Test
+  @NeedsFreshDriver
+  @SwitchToTopAfterTest
+  void setFilesUploadsTheFileContentsToTheServer() {
Evidence
Both new tests switch to the upload iframe and require exact body text. The existing equivalent
upload tests explicitly exclude Safari for this same assertion because Safari returns incorrect
frame-body text.

java/test/org/openqa/selenium/bidi/input/SetFilesCommandTest.java[131-155]
java/test/org/openqa/selenium/bidi/input/SetFilesCommandTest.java[158-188]
java/test/org/openqa/selenium/UploadTest.java[68-108]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new upload round-trip tests assert exact iframe body text, but Safari is known to return incorrect frame-body text for this upload flow. Without the existing Safari exception, these tests fail in Safari CI.

## Fix Focus Areas
- java/test/org/openqa/selenium/bidi/input/SetFilesCommandTest.java[20-23]
- java/test/org/openqa/selenium/bidi/input/SetFilesCommandTest.java[131-134]
- java/test/org/openqa/selenium/bidi/input/SetFilesCommandTest.java[158-161]

## Recommended Fix
Import the `SAFARI` browser constant and `NotYetImplemented`, then annotate both new round-trip test methods with `@NotYetImplemented(value = SAFARI, reason = "Returns wrong text of the frame body")`, matching the equivalent tests in `UploadTest`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Dismiss ↗ | View ↗


Grey Divider

Context sources
Review mode: Auto: 🚀 Fast: Localized additions to upload tests with contained, low-risk behavior.

Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread java/test/org/openqa/selenium/bidi/input/SetFilesCommandTest.java
@diemol
diemol merged commit af9e5cf into SeleniumHQ:trunk Oct 6, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-java Java Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants